Update UITableView to track changes precisely #1538

swankjesse · 2023-09-29T01:19:01Z

Previously we called reloadData() whenever anything changed in the underlying model.

With this change we fire precise events to the UI when inserts and removes happen in the source LazyList. This attempts to coalesce changes to itemsBefore() and itemsAfter() with adjacent changes and the beginning and end of the items list. To implement this the changes are buffered into an intermediate model before they are applied.

When a cell changes from being a placeholder to a loaded item, or the opposite, this does a cell content change without firing an event through the UITableView. To implement this we must track which cells are currently displaying placeholders. This PR includes a new datastructure, SparseList, to implement this tracking.

This PR also changes the number of placeholders that guest code offers to host code. I found experimentally that 20 placeholders was not enough for UITableView.

This PR changes LazyList from UICollectionView to
UITableView. This change was potentially unnecessary, though UITableView has fewer features that we don't need.

swankjesse · 2023-09-29T03:13:39Z

...t-widget/src/commonMain/kotlin/app/cash/redwood/lazylayout/widget/LazyListUpdateProcessor.kt

+
+    override fun remove(index: Int, count: Int) {
+      val last = edits.lastOrNull()
+      if (last is Edit.Remove && index in last.index - count until last.index + 1) {


Need to test fencepost logic on these.

veyndan

Just a drive-by review for now:

This PR changes LazyList from UICollectionView to UITableView. This change was potentially unnecessary, though UITableView has fewer features that we don't need.

Switching it back to UITableView means that LazyRow can no longer work (see #942 (comment)). I'd be very hesitant to break support for this!

veyndan · 2023-09-29T08:13:20Z

...lazylayout-uiview/src/commonMain/kotlin/app/cash/redwood/lazylayout/uiview/UIViewLazyList.kt

  ),
-) : WindowedLazyList<UIView>(UICollectionViewListUpdateCallback(collectionView)), ChangeListener {
+) : LazyList<UIView>, ChangeListener {


The goal of WindowedLazyList was to have UIViewLazyList and ViewLazyList to share the bulk of their implementations. They are fairly similar (e.g., view based, platform provides a recycling list view that allows atomic updates). The integration of WindowedLazyList with UIViewLazyList wasn't complete, namely that UICollectionViewListUpdateCallback didn't yet delegate to atomic updates.

What are your thoughts on continuing down that path vs switching approach as you did here?

There's this issue from last week that has more info: #1498

This new class started out as a copy-paste of WindowedLazyList, addressing problems on iOS by adding precise updates.

Are precise updates useful for RecyclerView? If precise updates will make RecyclerView faster, then we should follow-up by migrating RecyclerView to LazyListUpdateProcessor (and delete the old WindowedLazyList). If precise updates are not necessary for RecyclerView, then we should not do precise updates; it’s a bit of work to provide them.

Reading that issue makes it clear to me that this new thing will benefit both platforms. I’d still like to land it one-platform-at-a-time, mostly because our test coverage here is lacking.

That sounds good. Do you mind adding a TODO here (or manually create an issue) so that we remember that these two mechanisms should ideally be merged?

Here’s a tracking issue: #1552

veyndan · 2023-09-29T09:01:43Z

This PR also changes the number of placeholders that guest code offers to host code. I found experimentally that 20 placeholders was not enough for UITableView.

The underlying issue is probably #1500. Increasing to 30 placeholders increases the number of serialized views, which'll be unnecessary for the majority of lists.

dnagler · 2023-09-29T13:22:57Z

...lazylayout-uiview/src/commonMain/kotlin/app/cash/redwood/lazylayout/uiview/UIViewLazyList.kt

+    ): LazyListContainerCell {
+      val result = tableView.dequeueReusableCellWithIdentifier(
+        identifier = reuseIdentifier,
+        forIndexPath = NSIndexPath.indexPathForItem(index.convert(), 0.convert()),


nit: is it possible to use the more modern/bridged IndexPath type here?

I couldn’t find it in the Kotlin/Native APIs.

dnagler · 2023-09-29T13:24:19Z

...lazylayout-uiview/src/commonMain/kotlin/app/cash/redwood/lazylayout/uiview/UIViewLazyList.kt

+    override fun tableView(
+      tableView: UITableView,
+      numberOfRowsInSection: NSInteger,
+    ) = processor.size.toLong()


nit: You might want to add an assertion here that section == 0

dnagler · 2023-09-29T13:27:14Z

...lazylayout-uiview/src/commonMain/kotlin/app/cash/redwood/lazylayout/uiview/UIViewLazyList.kt

      prefetchingEnabled = true
+      rowHeight = UITableViewAutomaticDimension
+      estimatedRowHeight = 44.0


nit: is this needed? 44 is the default value since iOS 11

dnagler · 2023-09-29T13:29:35Z

...lazylayout-uiview/src/commonMain/kotlin/app/cash/redwood/lazylayout/uiview/UIViewLazyList.kt

-    } else {
-      UICollectionViewScrollDirectionHorizontal
-    }
+    // TODO: support horizontal LazyLists.


Do we plan to support this in the future, or can/should we punt this indefinitely? (Has this already been built on the Android side?)

I’d like to see what happens if I swap out UITableView for UICollectionView. If I’m lucky it’ll just be tracking slight differences in the APIs with no performance impact. I think I’ll be lucky.

@dnagler To answer your question directly:

We plan to support this in the future. We'll need this within Cash for things like the QAB, but also, LazyRow should be provided for external consumers (i.e., non Cash people), as these components are supposed to be the lazy counterparts to the existing Column/Row components.

Yes, it's already been built on the Android side.

dnagler · 2023-09-29T13:32:38Z

...lazylayout-uiview/src/commonMain/kotlin/app/cash/redwood/lazylayout/uiview/UIViewLazyList.kt

-    widgetView?.setFrame(this.contentView.bounds)
+
+    val widgetView = this.widgetView ?: return
+    widgetView.setFrame(bounds)


nit: since widgetView is a subview of contentView, I would probably do

contentView.setFrame(bounds) widgetView.setFrame(contentView.bounds)

instead, but it should have the same effect

we'll also definitely want to call widgetView.sizeToFit() here

After playing with this I’ve learned that contentView is disinterested in delegating to its children for stuff like sizing, so the cell needs to control both. I’m gonna keep it as-is for now ’cause it seems to be working!

swankjesse · 2023-09-29T13:41:13Z

@veyndan ooooh I like that technique to handle lack of placeholders. Right now the app crashes if there aren’t enough placeholders. I’d like to introduce this performance regression to fix that crash, and then later we can implement #1500 as an optimization.

Previously we called reloadData() whenever anything changed in the underlying model. With this change we fire precise events to the UI when inserts and removes happen in the source LazyList. This attempts to coalesce changes to itemsBefore() and itemsAfter() with adjacent changes and the beginning and end of the items list. To implement this the changes are buffered into an intermediate model before they are applied. When a cell changes from being a placeholder to a loaded item, or the opposite, this does a cell content change without firing an event through the UITableView. To implement this we must track which cells are currently displaying placeholders. This PR includes a new datastructure, SparseList, to implement this tracking. This PR also changes the number of placeholders that guest code offers to host code. I found experimentally that 20 placeholders was not enough for UITableView. This PR changes LazyList from UICollectionView to UITableView. This change was potentially unnecessary, though UITableView has fewer features that we don't need.

dnagler · 2023-09-29T15:15:13Z

After pairing on this (and seeing marked improvements over current performance!!), I'm in favor of landing this as-is and continuing to iterate, especially since this will be a great help to continuing our QA process on the iOS side (right now our QA team is testing a very broken experience).

swankjesse · 2023-10-03T18:55:23Z

@veyndan

I spent a couple days trying to switch this back to UICollectionView and I’ve been unsuccessful thus far. I can’t figure out how to get dynamic heights working without it exploding because it runs out of placeholders.

Could you please release your merge-block on this PR? We can do the UITableView to UICollectionView switch in a follow-up, but I’d like to unblock the stuff we need right now in Cash App.

veyndan

Stamping to unblock

...ood-lazylayout-compose/src/commonMain/kotlin/app/cash/redwood/lazylayout/compose/LazyList.kt

veyndan · 2023-10-04T09:25:31Z

...ood-lazylayout-compose/src/commonMain/kotlin/app/cash/redwood/lazylayout/compose/LazyList.kt

@@ -49,14 +49,14 @@ internal fun LazyList(
  val itemsBefore = remember(state.firstVisibleItemIndex) { (state.firstVisibleItemIndex - OffscreenItemsBufferCount / 2).coerceAtLeast(0) }
  val itemsAfter = remember(lastVisibleItemIndex, itemProvider.itemCount) { (itemProvider.itemCount - (lastVisibleItemIndex + OffscreenItemsBufferCount / 2).coerceAtMost(itemProvider.itemCount)).coerceAtLeast(0) }
  val scrollItemIndex = remember(state.scrollToItemTriggeredId) { ScrollItemIndex(state.scrollToItemTriggeredId, state.firstVisibleItemIndex) }
-  var placeholderPoolSize by remember { mutableStateOf(20) }
+  var placeholderPoolSize by remember { mutableStateOf(30) }


Can we add a TODO to drop this down to 20 again? It's really just blocked on iOS being unable to request more placeholders.

Done.

// TODO(jwilson): drop this down to 20 once this is fixed: // https://github.com/cashapp/redwood/issues/1551

veyndan · 2023-10-04T09:27:01Z

...lazylayout-uiview/src/commonMain/kotlin/app/cash/redwood/lazylayout/uiview/UIViewLazyList.kt

  ),
-) : WindowedLazyList<UIView>(UICollectionViewListUpdateCallback(collectionView)), ChangeListener {
+) : LazyList<UIView>, ChangeListener {


That sounds good. Do you mind adding a TODO here (or manually create an issue) so that we remember that these two mechanisms should ideally be merged?

veyndan · 2023-10-04T09:29:54Z

...lazylayout-uiview/src/commonMain/kotlin/app/cash/redwood/lazylayout/uiview/UIViewLazyList.kt

-    } else {
-      UICollectionViewScrollDirectionHorizontal
-    }
+    // TODO: support horizontal LazyLists.


@dnagler To answer your question directly:

We plan to support this in the future. We'll need this within Cash for things like the QAB, but also, LazyRow should be provided for external consumers (i.e., non Cash people), as these components are supposed to be the lazy counterparts to the existing Column/Row components.

Yes, it's already been built on the Android side.

swankjesse requested review from JakeWharton and veyndan September 29, 2023 01:19

swankjesse commented Sep 29, 2023

View reviewed changes

veyndan suggested changes Sep 29, 2023

View reviewed changes

dnagler reviewed Sep 29, 2023

View reviewed changes

squarejesse added 4 commits September 29, 2023 10:02

Restore accidentally-deleted file

9ea6e48

Restore some removed imports

8acf043

Address some PR feedback

db749b2

swankjesse force-pushed the jwilson.0922.uitableview branch from 2d86d1d to db749b2 Compare September 29, 2023 14:02

dnagler approved these changes Sep 29, 2023

View reviewed changes

veyndan approved these changes Oct 4, 2023

View reviewed changes

Address PR feedback

147898d

swankjesse merged commit 6d9d398 into trunk Oct 4, 2023
7 checks passed

swankjesse deleted the jwilson.0922.uitableview branch October 4, 2023 13:29

This was referenced Oct 4, 2023

Test validity of atomic updates given by WindowedLazyList #1497

Closed

On iOS, don't refresh all cells when items are inserted/removed #1498

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update UITableView to track changes precisely #1538

Update UITableView to track changes precisely #1538

swankjesse commented Sep 29, 2023

swankjesse Sep 29, 2023

veyndan left a comment •

edited

Loading

veyndan Sep 29, 2023

veyndan Sep 29, 2023

swankjesse Sep 29, 2023

swankjesse Sep 29, 2023

veyndan Oct 4, 2023

swankjesse Oct 4, 2023

veyndan commented Sep 29, 2023

dnagler Sep 29, 2023

swankjesse Sep 29, 2023

dnagler Sep 29, 2023

swankjesse Sep 29, 2023

dnagler Sep 29, 2023

swankjesse Sep 29, 2023

dnagler Sep 29, 2023

swankjesse Sep 29, 2023

veyndan Oct 4, 2023

dnagler Sep 29, 2023

dnagler Sep 29, 2023

swankjesse Sep 29, 2023

swankjesse commented Sep 29, 2023

dnagler commented Sep 29, 2023

swankjesse commented Oct 3, 2023

veyndan left a comment

veyndan Oct 4, 2023

swankjesse Oct 4, 2023

veyndan Oct 4, 2023

veyndan Oct 4, 2023

Update UITableView to track changes precisely #1538

Update UITableView to track changes precisely #1538

Conversation

swankjesse commented Sep 29, 2023

Choose a reason for hiding this comment

veyndan left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

veyndan commented Sep 29, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swankjesse commented Sep 29, 2023

dnagler commented Sep 29, 2023

swankjesse commented Oct 3, 2023

veyndan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

veyndan left a comment •

edited

Loading